-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
😍 Add module std #98
😍 Add module std #98
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool to see the modules patch can be used in Bazel!
"locale", | ||
"map", | ||
"mdspan", | ||
# "memory", # Bugged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I've checked my libc++ patch and the export of std::atomic
is indeed incorrect. We have not implemented atomiic smart pointers yet, that will export std::atomic
.
I had already fixed this locally, but I want to do other fixes before uploading a new revision in Phabricator.
fdb7910
to
24635c9
Compare
All preparations are now in rules_ll main. We could merge this pretty much at any point now. Technically we could add this without the patch fully working since it doesn't break existing code that doesn't It's hard to predict how long it'll take until upstream can merge this, as it needs to actually work to be committed into llvm main and the remaining issues seem to be the kinds of issues that one does not simply fix in a few minutes 😅 We should also keep in mind that modules don't work with remote execution ATM. This currently doesn't affect users that don't use modules, but if we build modules as part of the default toolchains it will become noticeable to everyone. We already have safeguards against this though, and remote execution is disabled for modules anyways. @SpamDoodler @jaroeichler WDYT? Do you want to play around with this as "highly experimental" feature in rules_ll main or would you prefer to wait until things are more stable? IMO, since the patch almost fully works, a compelling option would be to wait for @mordante's next revision mentioned above and then add this to rules_ll main (and probably bump the llvm commit to include all separate fixes). This way, users can preview it and "help" find bugs 🤣 We could make preview release and immediately create another release as soon as a fully working version is in llvm main. |
Pseudo-rebased the patch against the new llvm version. Modified the patch so that it only contains bugfixes and the cppm files. Apart from a buggy include this everything compiles now. |
It's finally here. Dayyyyyuuuummm I'm excited~
Nice to see this landed, I'm really curious to see how this works in practice. |
@mordante Thank you so much for your work on this, and also huge thanks to @ChuanqiXu9 who made it possible to build these targets with fully explicit dependencies ❤️ aec847b updates some examples to use the new std module. I'll cut a new release soon to make playing around with this easier for users. On the "regular" C++ side things work pretty much flawlessly so far (apart from some minor inconvenience warnings llvm/llvm-project#62844). Heterogeneous toolchains probably won't be able to use the std module for a while, but they can't use regular modules either 😅 |
See https://reviews.llvm.org/D144994 for progress the std module patch.